Skip to content

Eliminate command substitutions from skills and add permission audit#150

Merged
sebastientaggart merged 1 commit into
devfrom
feature/145-eliminate-command-substitutions
Apr 24, 2026
Merged

Eliminate command substitutions from skills and add permission audit#150
sebastientaggart merged 1 commit into
devfrom
feature/145-eliminate-command-substitutions

Conversation

@sebastientaggart

Copy link
Copy Markdown
Member

Eliminate $() command substitutions from skill shell commands that trigger agent permission prompts, and add a permission audit phase to /setup so agents can detect missing allow rules upfront.

Changes:

  • Replace cd "$(git rev-parse --show-toplevel)" in submit-for-review with a two-step pattern (get root, then cd to it) — removes the $() that Claude Code flags
  • Add permissions.yaml — hand-maintained manifest of shell command prefixes used across all skills
  • Add Phase 2 (permission audit) to /setup — reads the manifest, checks the agent's config, suggests missing allow rules. Advisory only.
  • Add validate_permissions() to sync.py --validate — catches drift between skills and the manifest
  • Add Bash(cd:*) to docs and example settings; normalize allow/deny rules to colon format

Issue #145

@sebastientaggart

Copy link
Copy Markdown
Member Author

Review Summary

Verdict: APPROVE

Findings

  • [WARNING] validate_permissions() only scans ```bash fenced code blocks (re.compile(r'```bash\n(.*?)```')) but several skills use plain ``` fences for their shell commands. In particular, qa.md (9 plain blocks with gh), review.md (1 plain block with gh), and submit-for-review.md (17 plain blocks with git, cd) will not be checked. The validator will silently pass even if those commands are removed from permissions.yaml. Consider matching both ```bash and plain ``` blocks, or at least documenting this limitation. (sync.py line 383)

  • [NOTE] validate_permissions is called with all_skill_files (line 497 in the diff) regardless of whether the user passed --skills to limit the scope. This is intentional for correctness (permissions are global), but it differs from how validate_placeholders respects the --skills filter. Worth a comment if intentional.

@sebastientaggart
sebastientaggart merged commit 8124b3a into dev Apr 24, 2026
6 checks passed
@sebastientaggart
sebastientaggart deleted the feature/145-eliminate-command-substitutions branch April 24, 2026 13:58
@sebastientaggart sebastientaggart mentioned this pull request May 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant